feat(authz): add roles table for Audit user page#106
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @jacobo-dominguez-wgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mphilbrick211
added
the
mao-onboarding
jacobo-dominguez-wgu
force-pushed
the
audit-user
branch
3 times, most recently
from
2151f75 to
ef7bf9b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
==========================================
+ Coverage 95.63% 95.70% +0.06%
==========================================
Files 68 69 +1
Lines 1466 1536 +70
Branches 319 321 +2
==========================================
+ Hits 1402 1470 +68
- Misses 62 64 +2
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobo-dominguez-wgu
force-pushed
the
audit-user
branch
2 times, most recently
from
808d584 to
045db70
Compare
jacobo-dominguez-wgu
requested review from
arbrandes,
bra-i-am,
dcoa,
jesusbalderramawgu and
rodmgwgu
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
I started by trying to test this. The page renders correctly:
But when I try to assign roles by clicking on "Assign Role", I get a bunch of errors seemingly stemming from 404s. Have the endpoints not been implemented, yet? I'm using the very latest edx-platform as of the time of this review.
If that's the case, please add the backend PRs that this depends on to the description. Otherwise, if there's some other way to test this, please outline the steps.
In the meantime, this is what Claude found when looking at the code. I think all points are worth addressing:
Bugs
1. navigate() called during render instead of in an effect
In src/authz-module/audit-user/index.tsx#L29-L31:
if (!user && !isLoadingUser) {
navigate(AUTHZ_HOME_PATH);
}This is a side effect executed during render. React will warn about state updates during render, and the component continues executing (building navLinks, columns, etc.) after the navigate call, which is wasted work at best and can lead to errors if it references state that becomes inconsistent mid-render.
The existing LibrariesUserManager.tsx handles the same pattern correctly:
useEffect(() => {
if (!isFetchingMember) {
if (!isLoadingTeamMember && !user?.username) {
navigate(teamMembersPath);
}
}
}, [isFetchingMember, isLoadingTeamMember, user?.username]);2. Pagination uses results.length instead of API count
In src/authz-module/audit-user/index.tsx#L74 and #L102:
const pageCount = userAssignments?.length
? Math.ceil(userAssignments.length / TABLE_DEFAULT_PAGE_SIZE) : 1;
...
itemCount={userAssignments?.length || 0}The table is configured with manualPagination, meaning the backend handles pagination and the frontend must tell the table how many total items exist. But itemCount and pageCount are derived from userAssignments.length - which is the current page of results, not the total. The API response includes a count field for the total, but the component destructuring on line 27 doesn't extract it:
const { data: { results: userAssignments } = { results: [] } } = useUserAssignedRoles(...);When a user has more than 10 role assignments, the table will display only the first page with no way to navigate further.
3. UserAccount type uses snake_case but camelCaseObject is applied
src/data/api.ts#L14-L17 applies camelCaseObject(data) to the API response, but the UserAccount type defines fields in snake_case (profile_image, date_joined, is_active, etc.). The actual runtime data has camelCase keys (profileImage, dateJoined, isActive), so the TypeScript types are incorrect.
This doesn't break the two fields actually used - username and email - since they're unchanged by camelCasing. But it's a type safety gap. The test mock data in src/data/hooks.test.tsx already uses camelCase (profileImage, dateJoined), confirming the mismatch. Either the type should use camelCase or camelCaseObject should be removed.
Issues
4. Route ordering in AuthZModule
In src/authz-module/index.tsx#L33-L36, the AUDIT_USER_PATH route is declared after the path="*" catch-all:
<Route path="*" element={<NotFoundError />} />
<Route path={ROUTES.AUDIT_USER_PATH} element={<AuditUserPage />} />React Router v6 ranks routes by specificity so this works correctly at runtime (/user/:username is more specific than *). But conventionally catch-all routes go last - having it in the middle makes the routing table harder to read and will mislead anyone unfamiliar with v6's ranking behavior.
5. CSS breakpoint gap for hr styling
The SCSS was refactored from a desktop-up approach to a mobile-first approach. On master, hr defaults to horizontal and switches to vertical at min-width-lg. In the new code (index.scss#L7-L10 and #L54-L63), it defaults to vertical and only switches to horizontal at max-width-sm. Screens between sm and lg breakpoints will now see vertical hr dividers where they previously saw horizontal ones. If this is intentional to match the new Figma designs, no issue - just flagging it since it affects the existing library pages too (via the class rename from .authz-libraries to .authz-module).
Hi @arbrandes, thanks for your review. The "Assign Role" button is not depending in any endpoint, it redirects to the role assignation wizard currently in development (here #109 and #111), I just added the redirection to that page but it does not exist yet thats why it shows 404, (Should I remove the button and add it when the wizard is done? So it does not become a blocker for this pr). Also I have addressed the rest of the points:
|
No need, but I still need a way to add data so I can see it working. How do I do that? |
jacobo-dominguez-wgu
force-pushed
the
audit-user
branch
from
fe1da23 to
c0d192e
Compare
Since the wizard is currently on development, you can still use the old library specific access manager and assign some roles there Or, can assign roles by directly using the api endpoint http://local.openedx.io:8000/api-docs/#/authz/authz_v1_roles_users_update I will add it to the testing instructions in the description. |
|
Can you give another look @arbrandes, @dcoa? |
|
Ok, thank you! I tried it out, and here were the results. With the version of the MFE in master, I successfully added all four roles to a user named 
I then checked out this PR and went to 
I am on an up-to-date edx-platform. Claude also caught a couple of remaining issues: 1. The previous review flagged that the export type ProfileImage = {
has_image: boolean;
image_url_full: string;
image_url_large: string;
image_url_medium: string;
image_url_small: string;
};
2. CSS class rename missed in The SCSS file renames <div className="authz-libraries">Since |
jacobo-dominguez-wgu
force-pushed
the
audit-user
branch
from
c0d192e to
6efc526
Compare
All the required backend prs are merged into main now for Update:
This one is addressed.
Solved Thanks @arbrandes. |
jacobo-dominguez-wgu
force-pushed
the
audit-user
branch
from
6efc526 to
8409d33
Compare
|
I can validate all the acceptance criteria, except this:
There is not page selector in the table. There is a reason for that? @jacobo-dominguez-wgu I also can see some errors that were addressed here #124 , so I suggest merge that and rebase this before approved. In general looks good. |
| export const useUserAssignedRoles = (username: string, querySettings: QuerySettings) => { | ||
| const result = useQuery<GetUserAssignmentsResponse, Error>({ | ||
| queryKey: authzQueryKeys.userRoles(username, querySettings), | ||
| queryFn: () => getUserAssignedRoles(username, querySettings), | ||
| staleTime: 1000 * 60 * 30, // refetch after 30 minutes | ||
| refetchOnWindowFocus: false, | ||
| }); | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
| export const useUserAssignedRoles = (username: string, querySettings: QuerySettings) => { | |
| const result = useQuery<GetUserAssignmentsResponse, Error>({ | |
| queryKey: authzQueryKeys.userRoles(username, querySettings), | |
| queryFn: () => getUserAssignedRoles(username, querySettings), | |
| staleTime: 1000 * 60 * 30, // refetch after 30 minutes | |
| refetchOnWindowFocus: false, | |
| }); | |
| return result; | |
| }; | |
| export const useUserAssignedRoles = (username: string, querySettings: QuerySettings) => useQuery<GetUserAssignmentsResponse, Error>({ | |
| queryKey: authzQueryKeys.userRoles(username, querySettings), | |
| queryFn: () => getUserAssignedRoles(username, querySettings), | |
| staleTime: 1000 * 60 * 30, // refetch after 30 minutes | |
| refetchOnWindowFocus: false, | |
| }); |
There was a problem hiding this comment.
there is a reason for this values?
staleTime: 1000 * 60 * 30, // refetch after 30 minutes
refetchOnWindowFocus: false,
There was a problem hiding this comment.
Thank you this change has been addressed!
about this
staleTime: 1000 * 60 * 30, // refetch after 30 minutes
I think is the time Jacobo considered correct for the data to be current
do you have any suggestions? or can we leave it this way?
There was a problem hiding this comment.
Please remove the refetchOnWindowFocus there was a unnecessary addition to the previous implementation. In terms of the stale time for now is okay but that is something that need to evaluate in the future with performance data.
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
Ok, I finally got this to work (I had to rebuild my openedx-dev image from scratch to get the latest openedx-authz), and it looks good! The only weird thing I saw is a blank role with asterisks:
Not a blocker, just curious. (I suspect it's just stale data.)
Thank you Adolfo, that issue is gonna be solved in one of my PR's later |
Thanks for your feedback, I checked and I see the page selector correctly but is not visible if we have less than 10 records 
|
dcoa
changed the title
…vements on the header
jacobo-dominguez-wgu
force-pushed
the
audit-user
branch
from
361e700 to
6766a57
Compare
jacobo-dominguez-wgu
moved this from Waiting on Author
to Ready for Review
in Contributions
|
Rebased and conflicts solved. @dcoa could you review and merge please? |
| const { formatMessage } = useIntl(); | ||
| const { username } = useParams(); | ||
| const navigate = useNavigate(); | ||
| const { isLoading: isLoadingUser, data: user } = useUserAccount(username ?? ''); |
There was a problem hiding this comment.
To avoid this username ?? '' conditional add enabled: !!username as a flag in the hook instead, so the API call is only trigger when a username exist. I'm not counting this as a blocker because it can be solved in a subsequent PR as a minor fix. @jacobo-dominguez-wgu
There was a problem hiding this comment.
Same with this behavior found by Claude:
the "not found" redirect treats any falsy user as "navigate away." If username is empty,
useUserAccount is disabled (data=undefined, isLoading=false) and the effect fires immediately — fine behaviorally, but if getUserAccount errors (5xx,
network), data is also undefined, so users get silently booted to home instead of seeing an error. Consider checking isError/error.status === 404
explicitly.
src/authz-module/audit-user/index.tsx:41-76 — columns / additionalColumns / navLinks are rebuilt on every render. fetchData is memoized but the
column arrays aren't; stabilize them with useMemo since DataTable uses referential identity.
src/authz-module/audit-user/index.tsx:79 — the debounced fetchData isn't cancelled on unmount. Add a cleanup effect calling fetchData.cancel() to avoid a setState-after-unmount warning.
There was a problem hiding this comment.
I have addressed all of them on the last commit, thanks @dcoa
There was a problem hiding this comment.
There was still a couple of comments left so they are in the issue #132
| const handleDelete = () => { | ||
| // TODO: Implement delete functionality | ||
| // eslint-disable-next-line no-console | ||
| console.log('Delete clicked for row:', row); |
There was a problem hiding this comment.
Please for traceability mention here the issue or PR that will implement this @jacobo-dominguez-wgu
| return ( | ||
| <ViewMoreLink | ||
| label={formatMessage(messages['authz.user.table.view_all_permissions.link.text'])} | ||
| // TODO: Implement view more functionality |
github-project-automation
Bot
moved this from Ready for Review
to Done
in Contributions
6 participants
Description
Creating the roles table on the audit user page. It includes:
It closes #86
Figma design
Pending things:
Unit testing.✅Integration with user assignment API when is ready Task - RBAC AuthZ - Implement user assignments endpoint for Admin Console openedx-authz#230.✅A couple of TODOs.✅Warning
Will be on draft status until the required endpoints are completed.This PR requires some the endpoints proposed in the section
"Proposed Endpoint for M2"openedx/openedx-authz#230
How to test it
Pre - requisites
A running local tutor instance.
It requires the endpoint created on this PR openedx/openedx-authz#230 (already merged)
Since the role assignation wizard is currently on development, we can still use the old library specific access manager and assign some roles there
http://apps.local.openedx.io:2025/admin-console/authz/libraries/:libraryIdor, can assign roles by directly using the api endpoint http://local.openedx.io:8000/api-docs/#/authz/authz_v1_roles_users_update
1 - Go to the new audit user page ->
http://apps.local.openedx.io:2025/admin-console/authz/user/:username(select a user with many roles assigned to get a more populated table) (NOTE: The team members table will redirect to this page, currently in progress #124)2 - Validate all the data loads properly, the UI matches the design and acceptance criteria is fulfilled.
NOTE: Expand all permissions and delete functionality are on different prs.
NOTE 2: The "Assign Role" button redirects to the new role assignation wizard (in development here #109 and #111).